Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #1119 csr remove unecessary code #1170

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Fix #1119 csr remove unecessary code #1170

wants to merge 20 commits into from

Conversation

JonasCir
Copy link

@JonasCir JonasCir commented Jan 10, 2025 •
edited
Loading

Fixes #1119

Hi,
this PR removes unnecessary elements from the generated project in case the user selects client-side rendering.

This PR:

  • renames assets to rendering method. In my opinion this is more readable and cleaner. The user's choice of the rendering method (i.e., client-side vs. server-side) ultimately decides how assets are served.
  • fixes all the mentioned items from Selecting Saas App with client side rendering creates unecessary code #1119, namely:
    • ViewEngine initializer not necessary in src/app.rs for CSR
    • src/initializers/view_engine.rs is not needed for CSR, the file and corresponding mod declarations and imports can be removed
    • asset folder can be removed for CSR
    • fluent-templates and unic-langid can be removed from Cargo.toml when using CSR

Please note that this changes the CLI output of the tool to use the term "rendering method" instead of "assets" when creating a new project.

I hand tested all the variants by running loco new and then inspecting the files in question.

Please let me know if there is anything unclear or if there are any open questions.

All the best,
Jonas

EDIT: I just noticed that I also need to adjust docs, READMEs, and the website. Will do this after a first review pass that signs of on the CLI changes.

JonasCir added 19 commits January 10, 2025 17:43
@JonasCir
refactor: rename asset tests to rendering_method tests
c09fde4
@JonasCir
refactor: rename AssetsOption to RenderingMethodOption
341b69f
@JonasCir
refactor: rename rendering_method template tests from asset to render…
be15960 
…ing_method
@JonasCir
refactor: rename remaining occurences of assets vars to rendering_met…
4908dff 
…hod in template tests
@JonasCir
refactor: rename Asset to RenderingMethod
53ef8e7
@JonasCir
refactor: rename Selections.asset -> rendering_method
f0a8160
@JonasCir
refactor: rename ArgsPlaceholder.asset -> rendering_method
8c4d874
@JonasCir
refactor: rename Settings.asset -> rendering_method. Adjust templatin…
7dce087 
…g as needed.

# Conflicts:
#	loco-new/base_template/config/development.yaml.t
#	loco-new/base_template/config/test.yaml.t
@JonasCir
refactor: rename asset scope var to rendering_method
9d9e7d4
@JonasCir
refactor: rename the remaing asset occurences
2b16fba
@JonasCir
refactor: rename the remaing asset occurences in main.rs
586d4cd
@JonasCir
refactor: rename the remaing asset occurences in tests
476ef5c
@JonasCir
fix: view_engine is not needed for csr
c60a557
@JonasCir
fix: asset folder is not needed for csr
6dbc1cd
@JonasCir
fix: cargo dependencies for csr
0d0eb50
@JonasCir
fix: use initializers module if there is any
7d60ac0
@JonasCir
style: run nightly cargo fmt
af375f3
@JonasCir
style: fix rhai_settings_extensions imports
016cd49
@JonasCir
docs: add documentation
9c472fa
kaplanelad
kaplanelad reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please revert the renaming of rendering_method? This field name is used in multiple places beyond the Loco CLI, and it would be better for us to maintain consistency across all references.

Another point, can you test the setup.rhai script??

@JonasCir
Copy link
Author

Hi @kaplanelad,
thank you for the comment, sure will do (this might still take a couple of days though).

Another point, can you test the setup.rhai script??

Can you briefly explain what you mean with that? I am happy to add tests for the rhai extension module I wrote, if this is what you mean?

P.S.: Out of curiosity, where else is the term "assets" used? If absolutely get the consistency requirement here. On the other hand, I think that using the term "assets" to differentiate between SSR and CSR (or doing nothing) is not optimal and has potential to confuse users when reading the documentation (and developers alike). If you are interested in this, we can discuss this separately (in a GH discussion) and I am happy to contribute a PR which fixes this across the project.

@kaplanelad
Copy link
Contributor

Regarding the setup.rhai tests, my goal is to ensure that they verify whether a copy operation occurred based on the settings. However, let's defer these improvements to a separate PR for now.

As for the naming, we can address potential changes in a separate PR to fully evaluate the impact across the entire project, including the CLI, framework, and documentation.

@JonasCir
Copy link
Author

Thanks @kaplanelad ! Happy to do this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaplanelad kaplanelad kaplanelad left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Selecting Saas App with client side rendering creates unecessary code
2 participants
@JonasCir @kaplanelad